Skip to content

fix(cloudkit): surface per-record modify errors instead of discarding them - #354

Open
MrJarnould wants to merge 1 commit into
timlaing:mainfrom
MrJarnould:fix/surface-cloudkit-modify-errors
Open

fix(cloudkit): surface per-record modify errors instead of discarding them#354
MrJarnould wants to merge 1 commit into
timlaing:mainfrom
MrJarnould:fix/surface-cloudkit-modify-errors

Conversation

@MrJarnould

Copy link
Copy Markdown

Proposed change

CloudKit answers 200 for a modify whose records were rejected, and puts the reason in the
record entry. CKModifyResponse.records is already typed
list[CKRecord | CKTombstoneRecord | CKErrorItem], so the error is modelled — but nothing
looked at it.

Every caller scanned for a CKRecord with a matching name, did not find one, and reported
something of its own invention. Invites raised "modify response missing record", which reads
like a response-shape problem. Photos filters errors out with an isinstance check. Reminders
does not look at all — a caller ignoring the return value would see a silent no-op.

modify() now raises CloudKitApiError naming every rejected record with Apple's code and
reason, carrying the response as the payload.

What this recovered, immediately

InvitesService.rsvp() reported:

RSVP modify response missing record '…_rsvp'

What Apple actually said was:

CONFLICT (record to insert already exists)

An entirely different problem, and it pointed straight at the cause: rsvp() chooses between
create and update by looking for the existing response in event.rsvps, and event hydration
was silently returning none of them. That is #350, fixed in #352. With both in place the write
takes effect — verified live on an event with no guests, GOINGMAYBE with a message, and
back.

So the chain was: a rejected query, discarded → empty hydration → wrong operation type → a
rejected write, also discarded → a message about response shape. Two swallowed errors between
the cause and the symptom, and this PR removes the second.

Scope

Raising rather than collecting, because no caller could have been relying on partial-failure
semantics: none of the three services inspect record errors at all today, so none can
distinguish a partial success from a total one. If a future caller wants per-record outcomes
for a non-atomic batch, the payload carries the full response and the behaviour can be
widened then.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New service (thank you!)
  • New feature (which adds functionality to an existing service)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests
  • Documentation or code sample

Additional information

Cut from main, touching only pyicloud/common/cloudkit/client.py and
tests/test_cloudkit_client.py. Independent of my other open PRs, though it and #352 are more
useful together.

Testing. 936 tests pass on Python 3.10, 3.11, 3.12, 3.13 and 3.14, run locally. Three are
new: a rejected record surfaces Apple's code and reason, a batch names every rejected record,
and a successful modify is unaffected.

No existing test needed changing, which is itself worth noting: nothing in the suite asserted
the old behaviour, because the old behaviour was to say nothing.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

  • Documentation added/updated to README

🤖 Generated with Claude Code

… them

CloudKit answers 200 for a modify whose records were rejected, and puts the
reason in the record entry. `CKModifyResponse.records` is already typed to
include `CKErrorItem`, but nothing looked: every caller scanned for a
`CKRecord` with a matching name, did not find one, and reported something
of its own invention.

So a rejected write surfaced as "modify response missing record", which
reads like a response-shape problem, and the only useful sentence in the
response was thrown away. Photos filters errors out with an isinstance
check; reminders does not look at all.

modify() now raises CloudKitApiError naming every rejected record with
Apple's code and reason, and carries the response as the payload.

Live proof of what this recovers. `InvitesService.rsvp()` reported "RSVP
modify response missing record". What Apple actually said was:

    CONFLICT (record to insert already exists)

which is a different problem entirely, and points at its real cause.

Closes timlaing#353

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 10 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 45a83548-fb16-4144-9aae-22fef14b517c

📥 Commits

Reviewing files that changed from the base of the PR and between 86c4bc9 and 48175a9.

📒 Files selected for processing (2)
  • pyicloud/common/cloudkit/client.py
  • tests/test_cloudkit_client.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant